Conversation
|
@soapun Can you help me with review here? I remeber that you built opentelemetry integration. Maybe you have an opinion on this change. Thank you) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #661 +/- ##
==========================================
- Coverage 82.91% 82.90% -0.01%
==========================================
Files 69 69
Lines 2756 2755 -1
==========================================
- Hits 2285 2284 -1
Misses 471 471 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
s3rius
left a comment
There was a problem hiding this comment.
This fix for the situation is actually correct.
The only 1 concern I have is that we will skip recording save events. In context of the system it might be okay.
|
@aticie, but can you please run |
be0d3d1 to
12aa51e
Compare
Agree on the concern. I think requeued task's span must be in the same trace as the original task span or even its child. Not sure if this is current behaviour. I guess it would be nice to be able to trigger |
Fixes an issue that happens with
OpenTelemetryMiddlewarewhen a message is requeued withContext.requeue().Context.requeue()usesself.broker.kickto kick a message to the broker which skips the Middlewarepre-sendandpost-sendinvocations that kicker does here:taskiq/taskiq/kicker.py
Lines 160 to 162 in ced1909
taskiq/taskiq/kicker.py
Lines 168 to 170 in ced1909
Messages that are requeued with the same context variables will raise an error on
post_save's.detach()because they are now running in a different async context.The message's lifecycle on OpenTelemetryMiddleware will be:
pre_send->message.labelsare injected with context herepost_sendpre_execute->message.labelsare extracted here and re-usedpost_executerequeue()happens -> skippingpre_sendandpost_send, therefore not renewing the context.pre_execute-> context inferred frommessage.labelsagainpost_executepost_save-> Detaching the stale context here. RaisesValueError: Token was created in a different contextThis change gets rid of
post_saveon OpenTelemetryMiddleware to detach the context onpost_executeinstead. Since, we are not actually doing anything database save related on the currentpost_savemethod, it seems fair to move everything underpost_executeas this is where the task execution actually ends.